Bring the docs in line with the tracked origin config - #1587
Conversation
README and docs/cache both still filed the origin nginx as living somewhere else. It has been in this repo since #1574, with a public-repo contract and a CI guard, and neither was mentioned anywhere a reader would look. docs/cache/nginx.md carried a hand-copied server block that had drifted into contradicting the tracked file: it showed X-Cache-Tier with always, which the vhost explicitly forbids because the upstream already sets that header and adding it emitted the header twice on every response, and it showed always on X-Cache-Status, which the real config deliberately omits. Replaced with the stable directives and a pointer to the file, so there is one copy rather than two that disagree. CLAUDE.md listed only apps and packages, and described pnpm test/lint/ typecheck as the checks, while three script audits gate the typecheck workflow and pnpm test runs none of them. The origin README now records why HEAD must not be answered in location /, and why the always flags are the other half of that fix rather than a tidy-up: those statuses were unreachable while every HEAD was a fabricated 200, so the deletion alone would have dropped HSTS from error responses. Also: the sitekey is inlined at build time, which the env template did not say, and the tenant nginx comment predates the Turnstile widget the form now carries.
|
Warning Review limit reached
Next review available in: 27 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent |
1 similar comment
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent |
PR Summary by QodoAlign docs with tracked origin nginx configuration
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91daabc45c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ⛔ **The vhosts are tracked: read `infra/origin/eu.ecency.com.conf` and | ||
| `us.ecency.com.conf` rather than a snippet here.** This section used to carry a | ||
| hand-copied `server { … }` block, which drifted and began contradicting the real config |
There was a problem hiding this comment.
Removing the $html_limited_bot { ... } block makes apps/web/src/specs/features/next-middleware/social-bot-metadata.spec.ts deterministically fail: lines 114–116 read this document and require that exact map before comparing it with htmlLimitedBots. Consequently every workflow running pnpm -r test, including PR-branch.yml, fails with “the $html_limited_bot map is missing”; retain a machine-readable copy or update the parity test to read another canonical source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in fa62b35. This was a real break and my judgement was wrong.
I read the whole ## Per-host config block as a stale hand-copy. Half of it was: the server { … } stanza had drifted into contradicting the tracked vhost, showing add_header X-Cache-Tier … always which infra/origin/eu.ecency.com.conf explicitly forbids because the upstream already sets it and adding it emitted the header twice on every response.
The map had not drifted, and is duplicated because it is enforced. social-bot-metadata.spec.ts:114-116 parses it out of this file and asserts term-for-term parity with htmlLimitedBots in next.config.js. So the block is not documentation of the config — it is a checked copy.
The part that should have stopped me: I wrote the invariant into my replacement text ("must stay in step with htmlLimitedBots") in the same edit that deleted the thing enforcing it. Documenting a rule while removing its enforcement is the exact failure the spec exists to prevent.
Restored, under a heading that says so rather than leaving the next reader to make the same call:
The bot map is CANONICAL here — do not remove it
⛔ This block is not documentation of the config, it IS a copy the test suite reads … Keep the fenced
nginxblock and the"~*( … )"shape — the test matches on them.
The stale server { } stanza stays removed, since that half of the finding was correct.
Verified: social-bot-metadata.spec.ts 5/5 (it was 4 passed / 1 failed, matching the repro), the whole next-middleware suite 165/165, and the full web suite 3090/3090.
| ```bash | ||
| node scripts/icon-tsx-audit.mjs --fail # icon sizing, see docs/icons.md | ||
| node scripts/icon-scss-audit.mjs # also fails if a retired SCSS rule reappears | ||
| node scripts/slim-entries-audit.mjs # feed payload invariants |
There was a problem hiding this comment.
When the slim-entry audit finds a violation, this documented command still exits successfully because scripts/slim-entries-audit.mjs only returns status 1 when --fail is present. Anyone following this new audit checklist can therefore miss the feed-payload violations that CI rejects; add --fail here to match the workflow command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both correct, fixed in fa62b35.
slim-entries-audit.mjs only exits 1 with --fail, so the command as I wrote it would report violations and exit 0 — the worst shape for a checklist, since it passes locally and fails in CI. And the heading said "Three" while listing four.
Now matched to typecheck.yml line for line, with the flags called out rather than left to be noticed:
node scripts/icon-scss-audit.mjs # also fails if a retired SCSS rule reappears
node scripts/icon-tsx-audit.mjs --fail # icon sizing, see docs/icons.md
node scripts/slim-entries-audit.mjs --fail # feed payload invariants
node scripts/origin-config-audit.mjs --self-test # prove the rules still fire, THEN enforce
node scripts/origin-config-audit.mjs --failwith "⛔ Copy the flags exactly: without --fail these REPORT and exit 0, so a violation passes locally and fails in CI."
Ordered as CI runs them, and icon-scss-audit deliberately has no flag because that is how the workflow invokes it.
Greptile SummaryThe PR aligns repository documentation and comments with the tracked origin configuration and current CI audit commands.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| docs/cache/nginx.md | Restores the test-consumed canonical bot map and replaces the stale copied vhost with references to tracked configuration. |
| CLAUDE.md | Documents the tracked origin tier and the audit commands currently enforced by the typecheck workflow. |
| infra/origin/README.md | Adds operational rationale for forwarding HEAD requests and retaining security headers on error responses. |
| .github/workflows/typecheck.yml | Adds explanatory comments to the existing origin-configuration audit step without changing workflow behavior. |
| README.md | Corrects cache-verification guidance and identifies the repository location and deployment model of origin vhosts. |
Reviews (3): Last reviewed commit: "Attribute the HEAD fix to the PR that ma..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Removing it broke the parity test that reads it. social-bot-metadata parses the $html_limited_bot map out of docs/cache/nginx.md and asserts term-for-term agreement with htmlLimitedBots in next.config.js, because drift there is invisible in dev and in production serves a browser-primed page to a crawler from the wrong cache namespace (#1257). I judged the whole block a stale hand-copy. Half of it was: the server stanza had drifted into contradicting the tracked vhost. The map had not, and is deliberately duplicated BECAUSE it is enforced. I even wrote the invariant into the replacement text while deleting the thing that enforces it, so the section now says plainly that the block is machine read and names the spec that reads it. Also: the audit list said three and listed four, and omitted --fail on slim-entries, so following it locally would report and exit 0 while CI failed. And the cache verification recipe claimed logged-in /discover always bypasses, contradicting cache-policy.ts (tier list, s-maxage 300) and the README's own note eighty lines above that only the mute-filtered tiers go private. Replaced with a tier that genuinely bypasses.
#1575 is the issue describing the fabricated 200; #1578 is the PR that removed it. The line read as though the issue did the work, which sends anyone following it to a report rather than a diff. Cites both now, since each answers a different question: the PR carries the change and the before/after measurements, the issue carries the reproduction. The main README already said #1578, so the two agree again.
|
Fixed in 3b545b3. Confirmed against the API rather than from memory: #1575 is the issue ("Origin vhosts answer every HEAD request with a fabricated 200") and #1578 is the PR ("Stop fabricating 200 for every HEAD at the origins"). The line read as though the issue did the work, which sends anyone following it to a report rather than a diff. Now cites both, because each answers a different question:
Swept the branch for the same mistake: the only other reference is
|
1 participant
Documentation and comments only, no code. Found by auditing the repo against what #1574, #1578 and #1579 actually changed.
The docs still say the origin nginx lives somewhere else
README.md: "Nginx and CF worker configs live in the infra repo" — the web origin vhosts have been in this repo since Track the web origin nginx config, with the public-repo contract enforced #1574.docs/cache/README.mdfiled nginx under "Operated outside this repo". It is tracked now, and applied by hand rather than by CI, which is the distinction worth stating.docs/cache/nginx.mdhad drifted into contradicting the tracked fileIt carried a hand-copied
server { … }block. Two lines were actively wrong:add_header X-Cache-Tier $upstream_http_x_cache_tier always;— the tracked vhost has an explicit NOTE forbidding this: the upstream already sets the header and nginx proxies it through, so adding it emitted the header twice on every response.alwaysonX-Cache-Status— the real config deliberately omits it, because that value is a cache diagnostic and not something we owe an error response.Replaced with the directives that are actually stable, plus a pointer to
infra/origin/*.conf. One copy instead of two that disagree. Also noted that the$html_limited_botmap lives athttplevel and is not tracked, and must stay in step withhtmlLimitedBotsinnext.config.js— if the app serves a blocking render to an agent the map does not classify, that response and a browser's share a cache entry.Corrected
eu/us/asia.ecency.comto the two live origins innginx.md; left the same stale claim incloudflare-worker.mdalone since the worker's origin list is its own subject.CLAUDE.mdwas missing a whole tracked tier and two thirds of the CI gatesapps/*andpackages/*;infra/originnow has an entry stating the public-repo contract and that CI does not deploy it.pnpm test/lint/typecheckas the checks. Three script audits also gatetypecheck.ymlandpnpm testruns none of them, so they are listed with what each enforces and why--self-testruns first.Rationale that only existed in a commit message
infra/origin/README.mdnow records whylocation /must not answer HEAD itself, and why thealwaysflags are the other half of that fix rather than a tidy-up: while every HEAD was a fabricated 200 those headers applied (200 is inadd_header's default status list), so deleting the block alone would have dropped HSTS from real 404/429/5xx. Plus the verification trap — through Cloudflare the bug reported as already fixed.typecheck.yml's new audit step was the only one in the file without a comment saying what it enforces.Smaller
apps/web/.env.templatedid not say the sitekey is inlined at build time, so setting it only in the runtime environment silently falls back to the literal.apps/self-hosted/hosting/nginx-multi-tenant.confjustified not routing tenant subscribes through public ecency.com partly because "a bot challenge would break the form outright". Since Require a bot check on anonymous newsletter subscribes, and offer the post prompt to anonymous readers #1579 the form carries a Turnstile widget of its own, solved in-page — a different thing from an edge interstitial the reader could not clear. Corrected in both copies; the block is byte-identical in two server blocks and nothing in CI compares them.Verification
origin-config-audit --self-test(34 cases) and--failboth pass — worth running here because the audit scans.mdas well, so a docs change can fail it.The nginx change is comment-only. I have not applied it to the blogs host; CI deploys that file.